Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Install state file #998

Merged
merged 47 commits into from
Feb 27, 2020
Merged

Install state file #998

merged 47 commits into from
Feb 27, 2020

Conversation

crubier
Copy link
Contributor

@crubier crubier commented Feb 26, 2020

What's the problem this PR addresses?

#973

How did you fix it?

A bit more brutally than in #990

Here we save the whole project state in one big file so there are no resolution to be done at run time.

@crubier
Copy link
Contributor Author

crubier commented Feb 26, 2020

@arcanis as discussed there #990 (comment)

I took a more brutal approach.

The interesting stuff is in Project.ts, in the generateInstallState and restoreInstallState functions, which simply serialize and restore the whole project state in a file.

However once again I am stuck. I thought this approach would be straightforward enough for me to manage it, but nope! It seems that restoring accessibleLocators , optionalBuilds , storedDescriptors , storedResolutions , storedPackages is not enough ? Or I dont do it right.

@arcanis
Copy link
Member

arcanis commented Feb 26, 2020

When dumping the loaded content I see that null (the null) is turned into null (the string). It seems this is because we're loading the file through the "failsafe" YAML schema, which doesn't support null as a value (TIL). Try storing it as JSON to see if that goes better.

@arcanis
Copy link
Member

arcanis commented Feb 26, 2020

Also of note: the file is 10M for our repository, I can't imagine what it will be for yours 😬 Maybe some basic format packing optimizations would be a good idea (I think ideally it shouldn't be much more than 1MB for our repo, given that the lockfile is 800KB).

@crubier crubier mentioned this pull request Feb 27, 2020
@arcanis
Copy link
Member

arcanis commented Feb 27, 2020

is there a way to speed up the setupResolutions using restoreInstallState?

The time in Project.find is likely the one needed to find the configuration, the workspaces, and read their manifests - I'm not sure there's a low-hanging fruit here (at the very least, it should be part of a separate PR).

Implement the check on the checksum of yarn.lock to invalidate the cache if the checksum don't match => @arcanis is there a yarn official way to do this or should I roll my own way to get the checksums?

There isn't at the moment, but I'm fine if you add a private field to Project to store the checksum of the lockfile when it's loaded in setupResolutions. Oh, and don't forget to hash a constant as well that so that we can force-refresh the cache on future releases with a different cache format.

I kept the skipVirtualResolution parameter that you made in #990 => @arcanis is it still needed in this MR or should I remove these changes ?

Not needed anymore, let's remove it.

Check what I did already in this MR (the zlib and v8.serialize) => @arcanis did I do it right, or is there a more yarn-kosher way to do what I did?

That seems about right. The code is formatted very differently from the rest of the app so I'll likely make a pass to uniformize it, but the logic itself looks fine.

@crubier
Copy link
Contributor Author

crubier commented Feb 27, 2020

Ok, finished!

I now write/control the checksum of the lockfile + installStateFileVersion before writing/reading the install state file. It adds 1-3ms depending on the project, so nothing much.

It looks like this

this.lockFileChecksum = hashUtils.makeHash(`${INSTALLSTATEFILE_VERSION}`,lockfileContent);

Let me know what you think @arcanis , change the code style if you like, and let's get this merged!

@crubier
Copy link
Contributor Author

crubier commented Feb 27, 2020

Results of the stress test: It's very good! It looks like this:

AFTER

Which is much better than it was before with #990 : (note the different vertical axis):

BEFORE

@crubier
Copy link
Contributor Author

crubier commented Feb 27, 2020

I had a look at the branch, it looks good to me

package.json Outdated Show resolved Hide resolved
It was nice for testing but a bit out of place in the end :-)

Co-Authored-By: Kristoffer K. <[email protected]>
@arcanis arcanis merged commit 92c3fae into yarnpkg:master Feb 27, 2020
@arcanis
Copy link
Member

arcanis commented Feb 27, 2020

🎉🎉🎉

Thanks @crubier!

@crubier
Copy link
Contributor Author

crubier commented Feb 27, 2020

Thank you so much @arcanis for supporting me through this! My first contribution to yarn, and a lifesaver for my company's tech team, reducing our install and build times by a lot! Best 🎉

eventualbuddha added a commit to decaffeinate/coffee-lex that referenced this pull request Aug 20, 2020
@crubier crubier mentioned this pull request Sep 9, 2020
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants